gh-156664: Fix interaction between free variables and comprehensions - #156691
gh-156664: Fix interaction between free variables and comprehensions#156691JelleZijlstra wants to merge 5 commits into
Conversation
carljm
left a comment
There was a problem hiding this comment.
Thanks! Codex found a couple regressions that this causes relative to main.
carljm
left a comment
There was a problem hiding this comment.
Looks good! Nice that this is actually a net LOC reduction, barring tests.
Codex found one remaining bug and a possible performance concern.
| } else { | ||
| if (!(_PyLocals_GetKind(co->co_localspluskinds, i) & CO_FAST_HIDDEN)) { | ||
| return i; | ||
| if (framelocalsproxy_hasval(frame->f_frame, co, i)) { |
There was a problem hiding this comment.
The comprehension's cell can be active but still empty before the first target assignment. In that state, this hasval() check skips it and directs a proxy write into the enclosing free-variable slot:
import sys
def values():
sys._getframe(1).f_locals["x"] = 42
yield 1
def outer():
x = 7
def inner():
[lambda: x for x in (x, values())[1]]
inner()
return x
print(outer())Main and 9c49003 print 7, while f2c0db42 prints 42. The write during iterator advancement escapes the comprehension and changes the enclosing variable.
| if (framelocalsproxy_hasval(frame->f_frame, co, i)) { | ||
| size++; | ||
| } | ||
| PyObject *snapshot = framelocalsproxy_snapshot(frame); |
There was a problem hiding this comment.
Not sure how much performance of framelocalsproxy matters in practice, but now even frames without comprehensions or duplicate names construct and destroy a full dictionary for len(proxy). The same snapshot cost also applies to key enumeration and the other views.
In matching --with-pydebug / -Og builds, Codex measured len(proxy) with 32 ordinary locals increasing from about 0.16 µs on main to 1.10 µs here; with 128 locals, it went from 0.52 µs to 4.39 µs. Key enumeration was roughly three times slower. These are microbenchmarks taking the minimum of three runs of 20,000 operations, on a frame with no comprehensions.
If this matters, we could maintain a fast path when duplicate bindings are impossible, especially for length, which previously required no container allocation?
Uh oh!
There was an error while loading. Please reload this page.